ref(httpx,httpx2): Move crumbs to integrations - #7149
Conversation
Move subprocess breadcrumb creation from the centralized `maybe_create_breadcrumbs_from_span` hook into the stdlib integration's `Popen.__init__` wrapper. This makes breadcrumbs work for both legacy spans and streamed spans, and removes the dependency on span internals.
…-redis-breadcrumbs-to-integration
| "reason": rv.reason_phrase, | ||
| } | ||
|
|
||
| if parsed_url and (not is_span_streaming_enabled or should_send_default_pii()): |
There was a problem hiding this comment.
The is_not_span_streaming_enabled part is there for continuity in transaction mode, where we don't care about should_send_default_pii before setting breadcrumb data.
Codecov Results 📊✅ 112111 passed | ⏭️ 6749 skipped | Total: 118860 | Pass Rate: 94.32% | Execution Time: 401m 48s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2487 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 90.17% 90.19% +0.02%
==========================================
Files 193 193 —
Lines 25337 25353 +16
Branches 9286 9294 +8
==========================================
+ Hits 22846 22866 +20
- Misses 2491 2487 -4
- Partials 1429 1428 -1Generated by Codecov Action |
Instead of parametrizing on sync/async httpx(2) client, split each test case into a sync and async variant, with the async variant as a proper `async def` function with `@pytest.mark.parametrize`. I did this because the tests routinely fail for me locally, and switching to using `pytest-asyncio` fixes that. The other reason is that for #7149, the breadcrumb tests don't simulate async very well, which leads to some scope problems and ultimately breadcrumbs not appearing on events in tests. I know this PR is not ideal since with the existing test duplication on span streaming/transaction tracing, we already have a LOT of test cases, and now many of them get an additional variant. But it does make them more resilient to random local (and I believe also CI) failures and we'll get rid of half of them on the new major branch.
| ) | ||
|
|
||
| add_http_breadcrumb(rv.status_code, breadcrumb_data) |
There was a problem hiding this comment.
HTTPX breadcrumbs are silently skipped when the request raises
When real_send raises an exception, the new add_http_breadcrumb call after the span block is unreachable, so no HTTP breadcrumb is recorded for failed requests. Previously Span.__exit__ created this crumb during cleanup regardless of exceptions.
Evidence
Span.__exit__intracing.pyunconditionally callsself.finish(), which in turn callsmaybe_create_breadcrumbs_from_span, so the old span-based mechanism always emitted a crumb—even when the request raised.- The new code places
breadcrumb_data = { ... }andadd_http_breadcrumb(...)after theif/elsespan blocks, so any exception propagated byreal_sendbypasses it entirely. - This means network errors, timeouts, or any other HTTP library exception inside the span block silently drop the breadcrumb, losing telemetry context that was previously available.
Identified by Warden · code-review · B7U-CFR
Description
Move breadcrumbs from the centralized, span-powered
maybe_create_breadcrumbs_from_spandirectly to the HTTPX and HTTPX2 integrations. (Put the two together in one PR since they're the same changeset.)Additional changes and context:
pytest-asyncio, as that simulates how the scopes behave live better than setting up an ad-hoc event loop.send_default_piibehavior).Issues
Reminders
uv run ruff.feat:,fix:,ref:,meta:)